Acquire with device code interval fix#3553
Conversation
samuelkubai
requested review from
pkanher617,
sameerag and
tnorling
as code owners
|
Fixed the failing test cases and linting errors and I think the pull request is ready for review by the team |
|
|
||
| if (response.body && response.body.error === Constants.AUTHORIZATION_PENDING) { | ||
| // user authorization is pending. Sleep for polling interval and try again | ||
| this.logger.info(response.body.error_description || "no_error_description"); |
There was a problem hiding this comment.
I would prefer something more meaningful here. Also, errors should be logged using this.logger.error.
There was a problem hiding this comment.
Replaced the "no_error_description" with "Authorization pending. Continue polling.", the response description is still most meaningful here as it is phrased like "AADSTS70016: OAuth 2.0 device flow error. Authorization is pending. Continue polling. Trace ID: 01707a0c-640b-4049-8cbb-ee2304dc0700 Correlation ID: 78b0fdfc-dd0e-4dfb-b13a-d316333783f6 Timestamp: 2020-03-26 22:54:14Z" with a lot of useful information for debugging.
There was a problem hiding this comment.
The message itself does not seem appropriate to categorize as an error message, I still think this.logger.info is the better categorization of the message.
|
|
||
| await delay(pollingIntervalMilli); | ||
| } else { | ||
| return response.body; |
There was a problem hiding this comment.
Can we add a verbose log message here?
| * @param t number | ||
| * @param value T | ||
| */ | ||
| function delay<T>(t: number, value?: T): Promise<T | void> { |
There was a problem hiding this comment.
Can this be defined somewhere more easily reusable?
There was a problem hiding this comment.
Also, this function and continuePolling are defined in different ways. I would propose they are both added as members of this class.
There was a problem hiding this comment.
+1 to making them class functions. delay can also be a utility function instantiated as needed.
There was a problem hiding this comment.
Moved the delay function to the TimeUtils utility class and made continuePolling a member of the class.
- make the continuePolling function a class member - move the delay function to the TimeUtils utility - add a more descriptive polling alternate message - add a verbose message on polling completion
4 participants
This is a proposal. I can't get this repo to work locally yet! Lerna doesn't get installed in my VM.
Something similar to this would:
Fix #3476